Fix volume plugin lazy init by yoonseo-han · Pull Request #28877 · podman-container-tools/podman

yoonseo-han · 2026-06-07T12:34:31Z

What this PR does

After reboot, Runtime.refresh() loads all volumes from the database. Previously, finalizeVolume attempted to contact the volume plugin for every plugin-backed volume and logged errors when the socket was not ready. This lead to volume unrelated commands to leave error logs during refrsh.

This defers plugin initialization until a volume operation actually needs the plugin. Volume create still validates the plugin eagerly.

Fixes #28110

Checklist

Ensure you have completed the following checklist for your pull request to be reviewed:

Certify you wrote the patch or otherwise have the right to pass it on as an open-source patch by signing all
commits. (git commit -s). (If needed, use git commit -s --amend). The author email must match
the sign-off email address. See CONTRIBUTING.md
for more information.
Referenced issues using Fixes: #00000 in commit message (if applicable)
Tests have been added/updated (or no tests are needed)
Documentation has been updated (or no documentation changes are needed)
All commits pass make validatepr (format/lint checks)
Release note entered in the section below (or None if no user-facing changes)

Does this PR introduce a user-facing change?

None

How is the code tested

Each test added in the e2e section tests the followign aspects

Test 1 (unrelated command after refresh does not log plugin errors for plugin volumes): Creatres plugin backed volumes in DB then forces refresh by deleting alive file. Runs network create and check if network succeeds and no plugin error logs appears
Test 2 (volume inspect with stopped plugin errors without finalize spam): Creates plugin backed volume, stops the plugin and runs volume inspect. Check if instpect fails with correct error and no finalize spam appears
Test 3 (volume ls with stopped plugin fails on inspect without finalize spam): Creates plugin backend volume, stops the plugin and runs volume ls. Check if ls fails with correct error and no finalize spam appears

Luap99 · 2026-06-09T10:38:55Z

+		// Create the libpod alive file (runtime already initialized).
+		init := podmanTest.Podman([]string{"volume", "ls", "-q"})
+		init.WaitWithDefaultTimeout()
+		Expect(init).Should(ExitCleanly())


that does nothing since the prior commands already did this

Luap99 · 2026-06-09T10:39:27Z

+
+		alivePath := filepath.Join(podmanTest.TmpDir, "alive")
+		err = os.Remove(alivePath)
+		Expect(err).ToNot(HaveOccurred())


That is somewhat dangerous and should need at least comments why this is done

Luap99 · 2026-06-09T10:41:17Z

+		netName := "refresh-net-" + stringid.GenerateRandomID()
+		session := podmanTest.Podman([]string{"--log-level=error", "network", "create", netName})
+		session.WaitWithDefaultTimeout()
+		Expect(session.ExitCode()).To(Equal(0))
+		Expect(session.ErrorToString()).NotTo(ContainSubstring("some functionality may not be available"))
+		Expect(session.ErrorToString()).NotTo(ContainSubstring("uses volume plugin"))


negative checks are not good and cover very little. The correct thing is to check for an empty stderr wich ExitCleanly does by default

Also do not create a new network, this will get leaked on errors. you can run any command here to trigger the refresh.

Luap99 · 2026-06-09T10:42:40Z

+		inspect := podmanTest.Podman([]string{"volume", "inspect", volName})
+		inspect.WaitWithDefaultTimeout()
+		Expect(inspect).To(ExitWithErrorRegex(125, "cannot inspect"))
+		Expect(inspect.ErrorToString()).NotTo(ContainSubstring("some functionality may not be available"))


again no negative checks

Luap99 · 2026-06-09T10:43:03Z

+		init := podmanTest.Podman([]string{"volume", "ls", "-q"})
+		init.WaitWithDefaultTimeout()
+		Expect(init).Should(ExitCleanly())
+
+		podmanTest.StopContainer(ctrName)
+
+		ls := podmanTest.Podman([]string{"--log-level=error", "volume", "ls", "-q"})
+		ls.WaitWithDefaultTimeout()
+		Expect(ls).To(ExitWithErrorRegex(125, "cannot inspect"))
+		Expect(ls.ErrorToString()).NotTo(ContainSubstring("some functionality may not be available"))


same thing here, and just combine this with the test case above to save code and time

Luap99 · 2026-06-09T10:44:08Z

+
+		vol1 := "refreshVol1-" + stringid.GenerateRandomID()
+		vol2 := "refreshVol2-" + stringid.GenerateRandomID()
+		create1 := podmanTest.Podman([]string{"volume", "create", "--driver", pluginName, vol1})


not for all the commands that should exited with error all new test code should be using PodmanExitCleanly instead to reduce boiler plate

Luap99 · 2026-06-09T10:44:32Z

+	v.volumePlugin = sync.OnceValues(func() (*plugin.VolumePlugin, error) {
+		return p, nil
+	})
+}


That seems incorrect and hard to use correctly, it is at least not how sync.OnceValues is supposed to be used.

You assign a new function pointer to a struct field, calling these in parallel is thus not safe and also more complicated to use.

The proper way should be to not have either of these fucntions. When retrieving the volume from the db the volumePlugin field should already be set with the sync.OnceValues in place.

And then all callers need to just call v.volumePlugin()

Luap99 · 2026-06-09T10:45:36Z

Also please squash the commits, we prefer test and code in one commit.

yoonseo-han · 2026-06-13T13:28:28Z

Hi @Luap99

Thanks for the review. Made some respective changes based on your comments and squashed commits.

Would very much appreciate another check of yours

Luap99 · 2026-06-19T13:57:45Z

Sorry for the delay, I have not done a deep look but this looks about right on a quick skim.

You have a merge conflict though so please rebase the PR to latest main to resolve that conflict

Add e2e regression tests for refresh and lazy plugin use paths. Signed-off-by: Yoonseo Han <yooncer00@gmail.com>

packit-as-a-service · 2026-06-20T06:44:02Z

[NON-BLOCKING] Packit jobs failed. @containers/packit-build please check. Everyone else, feel free to ignore.

yoonseo-han · 2026-06-20T07:02:36Z

/packit test

yoonseo-han · 2026-06-20T08:15:27Z

No worries.

Rebased onto latest main and fixed any conflicts. Some build fails that doesn't look related to this PR appeared. Mind taking a look when you have a moment?

Thanks
@Luap99

yoonseo-han force-pushed the fix-volume-plugin-lazy-init branch from 056fba8 to 4795354 Compare June 7, 2026 14:42

Luap99 requested changes Jun 9, 2026

View reviewed changes

yoonseo-han force-pushed the fix-volume-plugin-lazy-init branch from 4795354 to 5f14dc3 Compare June 13, 2026 12:59

libpod: defer volume plugin init when loading volumes from DB

914e518

Add e2e regression tests for refresh and lazy plugin use paths. Signed-off-by: Yoonseo Han <yooncer00@gmail.com>

yoonseo-han force-pushed the fix-volume-plugin-lazy-init branch from 5f14dc3 to 914e518 Compare June 20, 2026 06:07

Conversation

yoonseo-han commented Jun 7, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What this PR does

Checklist

Does this PR introduce a user-facing change?

How is the code tested

Uh oh!

Luap99 Jun 9, 2026

Choose a reason for hiding this comment

Uh oh!

Luap99 Jun 9, 2026

Choose a reason for hiding this comment

Uh oh!

Luap99 Jun 9, 2026

Choose a reason for hiding this comment

Uh oh!

Luap99 Jun 9, 2026

Choose a reason for hiding this comment

Uh oh!

Luap99 Jun 9, 2026

Choose a reason for hiding this comment

Uh oh!

Luap99 Jun 9, 2026

Choose a reason for hiding this comment

Uh oh!

Luap99 Jun 9, 2026

Choose a reason for hiding this comment

Uh oh!

Luap99 commented Jun 9, 2026

Uh oh!

yoonseo-han commented Jun 13, 2026

Uh oh!

Luap99 commented Jun 19, 2026

Uh oh!

packit-as-a-service Bot commented Jun 20, 2026

Uh oh!

yoonseo-han commented Jun 20, 2026

Uh oh!

yoonseo-han commented Jun 20, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

yoonseo-han commented Jun 7, 2026 •

edited

Loading